Skip to content

Honor an explicit MinimumPreserved=0 in compaction strategies instead of collapsing to the default - #686

Open
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:honor-explicit-minimum-preserved-zero
Open

Honor an explicit MinimumPreserved=0 in compaction strategies instead of collapsing to the default#686
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:honor-explicit-minimum-preserved-zero

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

The four compaction strategies (TruncationStrategy, SlidingWindowStrategy, SummarizationStrategy, ToolResultStrategy) computed their minimum-preserved floor with:

minimumPreserved := cmp.Or(max(strategy.MinimumPreserved*, 0), default)

Because max(0, 0) == 0 and cmp.Or(0, default) == default, this folds three distinct inputs into the type default (truncation 32, sliding window 1, summarization 8, tool result 16):

  • unset (the intended default case) — correct
  • an explicit 0 — silently overridden by the default
  • a negative value — silently overridden by the default

So a caller who set MinimumPreservedGroups: 0 to preserve nothing got the default instead, and the aggressive preserve-none configuration was inexpressible. Only positive values worked.

This changes each MinimumPreserved* field to *int: a nil config uses the default, while a non-nil value is honored as-is with a negative clamped to 0:

minimumPreserved := default
if strategy.MinimumPreserved* != nil {
    minimumPreserved = max(*strategy.MinimumPreserved*, 0)
}

Why

The .NET port treats this as a nullable int and applies EnsureNonNegative(value) => Math.Max(0, value), so an explicit 0 disables the floor and negatives clamp to 0. The Go port silently discarded the value instead, leaving no equivalent for the preserve-none case. This restores cross-SDK parity.

Tests

  • Added TestTruncationStrategy_ExplicitZeroPreservesNone: an index with 5 non-system groups, nil Trigger/Target, and MinimumPreservedGroups: ptr(0) now compacts and excludes all removable non-system groups rather than preserving the default 32.
  • Added TestSlidingWindowStrategy_NegativeMinimumClampsToZero: a negative floor clamps to 0 (excluding all turns) instead of using the default 1.
  • Updated existing call sites (contextwindow.go, the step18 example) and existing tests to the pointer field type.

go build ./..., go vet ./agent/compaction/..., and go test ./agent/compaction/... all pass.

@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 24, 2026 01:30
Copilot AI review requested due to automatic review settings July 24, 2026 01:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the compaction strategies’ “minimum preserved” floors so callers can explicitly set 0 (preserve none) and have it honored, while keeping defaults when the value is unset. This aligns the Go SDK behavior with the .NET port by making the minimum-preserved fields nullable and clamping negative values to 0.

Changes:

  • Changed MinimumPreservedGroups / MinimumPreservedTurns fields from int to *int across compaction strategies to distinguish “unset” vs “explicit 0”.
  • Updated compaction logic to use defaults when the pointer is nil, otherwise clamp the provided value to >= 0.
  • Updated examples and tests; added new tests covering explicit 0 and negative clamping behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
examples/02-agents/agents/step18_compaction_pipeline/main.go Updates example strategy config to use pointer-valued minimum-preserved fields and adds a local ptr helper.
agent/compaction/truncation.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives.
agent/compaction/toolresult.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives.
agent/compaction/summarization.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives while keeping existing prompt/default behavior.
agent/compaction/slidingwindow.go Makes MinimumPreservedTurns nullable and honors explicit 0 / clamps negatives.
agent/compaction/index_test.go Updates tests to use pointer-valued MinimumPreservedGroups.
agent/compaction/contextwindow.go Updates internal strategy wiring to provide pointer-valued minimum-preserved config.
agent/compaction/compaction_test.go Updates tests, adds new coverage for explicit 0 and negative clamping, and adds a ptr helper for tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PratikDhanave
PratikDhanave force-pushed the honor-explicit-minimum-preserved-zero branch from 68767cd to 1ce19e4 Compare July 24, 2026 01:40
@github-actions github-actions Bot added the public-api-change Pull Request changes public APIs label Jul 24, 2026
@github-actions

This comment has been minimized.

The four compaction strategies computed their preserve floor with
cmp.Or(max(field, 0), default), which folds unset, explicit 0, and
negative values all into the type default (truncation 32, sliding
window 1, summarization 8, tool result 16). A caller who set
MinimumPreserved* to 0 to preserve nothing silently got the default
instead, and there was no way to express the preserve-none config.

Change each MinimumPreserved* field to *int so an unset (nil) config
uses the default while an explicit value is honored as-is, clamping a
negative to 0. This mirrors the .NET port, which uses a nullable int
and EnsureNonNegative(value) => Math.Max(0, value).
@PratikDhanave
PratikDhanave force-pushed the honor-explicit-minimum-preserved-zero branch from 1ce19e4 to 083c194 Compare July 24, 2026 09:34
@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 24, 2026
@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Cross-Repo Parity Review

Verdict: ✅ Parity restored — no issues found.

This PR fixes a Go-only bug where setting MinimumPreserved* to 0 in a struct literal was silently collapsed to the type default via cmp.Or(max(0,0), default). The upstream .NET implementations (TruncationCompactionStrategy, SlidingWindowCompactionStrategy, SummarizationCompactionStrategy, ToolResultCompactionStrategy in dotnet/src/Microsoft.Agents.AI/Compaction/) use constructor parameters with default values and EnsureNonNegative, so passing 0 has always been expressible in .NET. The Go port could not represent that case due to zero-value ambiguity in struct literals.

API Shape: The change from int to *int for these fields is idiomatic Go for the absence of default parameter values. Semantically: nil maps to the language default; non-nil (including 0) is honored with negatives clamped to 0 — exactly mirroring .NET's EnsureNonNegative. No Python equivalent of compaction strategies was found in the upstream Python packages, so there is no Python parity concern.

Labels: public-api-change is appropriate (exported struct field types changed). parity-approved is appropriate — this change restores rather than diverges from upstream semantics.

Generated by Go API Consistency Review Agent · sonnet46 · 19.5 AIC · ⌖ 5.6 AIC · ⊞ 5.7K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues public-api-change Pull Request changes public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants